feat: Add allSiteIdsByTier and allByEnrollmentAndTier collection meth… - #1569
Conversation
|
This PR will trigger a minor release when merged. |
Monorepo uses npm workspaces with a single root lockfile; this per-package lockfile was added by mistake.
There was a problem hiding this comment.
Hey @sandsinh,
Verdict: Request changes - one deduplication gap to address before merge.
Complexity: MEDIUM - medium diff, new collection methods following existing patterns.
Changes: Adds tier-based site enrollment lookup methods (allSiteIdsByTier and allByEnrollmentAndTier) with full test coverage (8 files).
Must fix before merge
- [Important] Duplicate site IDs not deduplicated before batch fetch -
packages/spacecat-shared-data-access/src/models/site-enrollment/site-enrollment.collection.js:79(details inline)
Non-blocking (2): minor issues and suggestions
- nit: Inconsistent input validation -
allSiteIdsByTierusesif (!tier)whileallByEnrollmentAndTierusesif (!hasText(tier)). The falsy check accepts whitespace-only strings; consider aligning tohasTextfor consistency -site-enrollment.collection.js:59 - nit: TypeScript declarations for
allSiteIdsByProductCodeandallByEnrollmentProductCodeare added to the.d.tsfiles but their implementations are not visible in this diff. Confirm these are pre-existing methods getting their missing types backfilled alongside the new code -site-enrollment/index.d.ts:9,site/index.d.ts:66
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 2m 41s | Cost: $4.78 | Commit: 71632b2245ae3b16aa6e6f6926c90ebf477b7b20
If this code review was useful, please react with 👍. Otherwise, react with 👎.
| this.log.error(`[SiteEnrollmentCollection] Failed to query site_enrollments by tier - ${error.message}`, error); | ||
| throw new DataAccessError('Failed to query site_enrollments by tier', { entityName: 'SiteEnrollment', tableName: 'site_enrollments' }, error); | ||
| } | ||
|
|
There was a problem hiding this comment.
issue (blocking): allSiteIdsByTier returns raw (data || []).map((row) => row.site_id) without deduplication. If a site is enrolled in multiple entitlements at the same tier (e.g., two PAID entitlements for different product codes when no productCode filter is supplied), the query returns duplicate site_id values. These duplicates flow into allByEnrollmentAndTier and cause redundant batchGetByKeys lookups - wasted I/O on the same primary keys.
The existing allSiteIdsByProductCode sibling has the same pattern but is less likely to produce duplicates in practice (unique on (site_id, entitlement_id) plus a specific product code narrows to one entitlement per site). Tier is a broader classifier and more likely to hit this.
Fix:
return [...new Set((data || []).map((row) => row.site_id))];
MysticatBot
added
ai-reviewed
- Dedupe site IDs in allSiteIdsByTier before batch fetch (duplicate rows possible when a site holds multiple entitlements at the same tier without a productCode filter) - Align allSiteIdsByTier's tier validation to hasText for consistency with allByEnrollmentAndTier Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Hey @sandsinh,
Verdict: Approve - all prior findings addressed, no new blocking issues.
Complexity: MEDIUM - medium diff, new collection methods following existing patterns.
Changes: Adds tier-based site enrollment lookup methods with deduplication fix and full test coverage (8 files).
Previously flagged, now resolved
- Duplicate site IDs not deduplicated before batch fetch - now uses
[...new Set(...)]inallSiteIdsByTier - Inconsistent input validation (
!tiervshasText(tier)) - both methods now usehasText(tier) - TypeScript declarations for
allSiteIdsByProductCodeandallByEnrollmentProductCodebackfilled alongside new methods
Non-blocking (1): minor issues and suggestions
- suggestion: The pre-existing
allSiteIdsByProductCodemethod (line 47) still uses(data || []).map((row) => row.site_id)without Set-based deduplication. The same duplicate-site-ID scenario is possible there. Consider aligning it with the newallSiteIdsByTierpattern in a follow-up -site-enrollment.collection.js:47
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 43s | Cost: $3.80 | Commit: ecdd326f638c552579cd4ada7212322674976d54
If this code review was useful, please react with 👍. Otherwise, react with 👎.
## [@adobe/spacecat-shared-data-access-v4.10.0](https://github.com/adobe/spacecat-shared/compare/@adobe/spacecat-shared-data-access-v4.9.0...@adobe/spacecat-shared-data-access-v4.10.0) (2026-07-20) ### Features * Add allSiteIdsByTier and allByEnrollmentAndTier collection meth… ([#1569](#1569)) ([3ac585b](3ac585b))
|
🎉 This PR is included in version @adobe/spacecat-shared-data-access-v4.10.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
#1569) ## What Adds two new collection methods to `spacecat-shared-data-access` for querying sites by entitlement tier: - `SiteEnrollmentCollection.allSiteIdsByTier(tier, productCode?)` — single JOIN query against `site_enrollments`/`entitlements` returning matching `siteId`s, optionally narrowed to a product code. - `SiteCollection.allByEnrollmentAndTier(tier, productCode?, options?)` — chains through `SiteEnrollmentCollection` via `entityRegistry` and batch-fetches full `Site` objects for those IDs. ## Why Callers need to look up sites by entitlement tier (e.g. `PAID`, `FREE_TRIAL`, `PLG`), optionally scoped to a product code (e.g. `LLMO`), without fetching every enrollment and filtering in memory. ## Testing Unit and integration tests added for both new methods (`site.collection.test.js`, `site.test.js`, `site-enrollment.collection.test.js`, `site-enrollment.test.js`). ## Related Issues SITES-43769 --------- Co-authored-by: Sandesh Sinha <sandsinh@Sandeshs-MacBook-Pro.local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
## [@adobe/spacecat-shared-data-access-v4.10.0](https://github.com/adobe/spacecat-shared/compare/@adobe/spacecat-shared-data-access-v4.9.0...@adobe/spacecat-shared-data-access-v4.10.0) (2026-07-20) ### Features * Add allSiteIdsByTier and allByEnrollmentAndTier collection meth… ([#1569](#1569)) ([3ac585b](3ac585b))
3 participants
What
Adds two new collection methods to
spacecat-shared-data-accessfor querying sites by entitlement tier:SiteEnrollmentCollection.allSiteIdsByTier(tier, productCode?)— single JOIN query againstsite_enrollments/entitlementsreturning matchingsiteIds, optionally narrowed to a product code.SiteCollection.allByEnrollmentAndTier(tier, productCode?, options?)— chains throughSiteEnrollmentCollectionviaentityRegistryand batch-fetches fullSiteobjects for those IDs.Why
Callers need to look up sites by entitlement tier (e.g.
PAID,FREE_TRIAL,PLG), optionally scoped to a product code (e.g.LLMO), without fetching every enrollment and filtering in memory.Testing
Unit and integration tests added for both new methods (
site.collection.test.js,site.test.js,site-enrollment.collection.test.js,site-enrollment.test.js).Related Issues
SITES-43769